-
Notifications
You must be signed in to change notification settings - Fork 1
New Squin statements. #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
@weinbe58 is there a reason we don't want broadcast in the wire dialect as well? Or is the idea that there will be some lowering from a qubit broadcast to a bunch of wire applications? |
@@ -138,6 +146,18 @@ class PauliOp(ConstantUnitary): | |||
pass | |||
|
|||
|
|||
@statement(dialect=dialect) | |||
class CliffordString(ConstantUnitary): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a CliffordString supposed to represent? PauliString has specific uses like defining a multi-qubit operator for measurement, control, or phase rotation. I am not familiar with "Clifford string".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to represent a product of Clifford operators not just Pauli. But if this is confusing we don't need to have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clifford include also CX, do we also want product of mixing 2Q and 1Q?
Some initial friction points/anticipated changes I can immediately see in translating this to stim - The original `PauliChannel` definition from @kaihsin can take a variable number of float arguments, but in the stim dialect there are explicit fields for each probability of a 1Q/2Q pauli operator being applied. My feeling is that this should be reflected on the squin side as just something like `1QPauliChannel` and `2QPauliChannel` with explicit fields. I don't think we'd lose any generality here along with the added benefit that it would be much harder for a user to misinput information (the default definition assumes you plug in either 3 or 15 values via varargs but it seems to easy to make a mistake here) - ~~If we DO want varargs I'm not quite sure how to nicely feed this to the kirin `@wraps` decorator~~ - From #200 , I see it could be possible to not have to provide an operator for `basis` and instead use a `CliffordString` (although I'd have to restrict CliffordString even further, to just the Pauli Operators. Would it be worth having something like a `PauliString`? Or is that silly?) cc: @weinbe58 @Roger-luo @kaihsin --------- Co-authored-by: Phillip Weinberg <[email protected]>
This PR is addressing the new squin statements. However, after discussion with @kaihsin and @Roger-luo we have slightly modified the semantics to make things a bit cleaner:
Boradcast
is now an appliy-like statement, see Broadcast should be an Apply-like statement. #206PualiString
is nowCliffordString
to enable better rewrites to STIM.